Added E2E tests using the SQLiteKIM #529
Conversation
Closes parallaxsecond#516 Signed-off-by: Matt Davis <matt.davis@arm.com>
There was a problem hiding this comment.
Looks good for me! I think the important point in my comment is about adding some persistence test for the SQL KIM.
Also thinking more and more that our test script is getting more and more complicated and is all in one line of bash... We might want at some point to switch to something more maintainable and scalable.
| wait_for_service | ||
|
|
||
| echo "Execute all-providers sqlite-kim normal tests" | ||
| RUST_BACKTRACE=1 cargo test $TEST_FEATURES --manifest-path ./e2e_tests/Cargo.toml all_providers::normal |
There was a problem hiding this comment.
I am wondering, if we are going to promote the SQL KIM as the default KIM from now on, if we should change all config files to use the SQL KIM and add that if case for the legacy KIM.
Also, we might need stability/persistence tests for it: that old mappings stored with that KIM can still be read in current PRs. Check the "per provider key mappings tests" here
There was a problem hiding this comment.
Yes, I was also wondering whether we should make it the default. The reason it is currently only the subset of tests (all_providers::normal) is that quite a few of the E2E tests currently rely on the fact that the OnDiskKIM is able to store keys with the same name in different providers. Tests that rely on this logic obviously fail. Quite a few of the E2E tests would therefore need refactoring to account for this, hence why I went with this approach for now 😅 better to have some E2E tests that none.
As for stability tests I already have an issue tracking this here that hopefully somebody will pick up, or me when I'm back.
There was a problem hiding this comment.
Oh nice that you already thought about stability in an issue 👍
Understand about the refactoring needed for the tests. Could you please open an extra-issue about "Making the SQL KIM the default" to track what needs to be done for it?
I am fine to merge this PR then.
Runs the current E2E
all_providers::normaltests. Doesn't run multitenancy etc.Closes #516
Signed-off-by: Matt Davis matt.davis@arm.com